Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move System.Object serialization to ObjectConverter #54436

Merged
2 commits merged into from
Jun 23, 2021
Merged

Move System.Object serialization to ObjectConverter #54436

2 commits merged into from
Jun 23, 2021

Conversation

eiriktsarpalis
Copy link
Member

Continuing the backporting efforts of #54420, this PR brings a change from #54328 that requires review in isolation:

Potential breaking change

The changes introduce a deliberate breaking change when it comes to the handling of new object() values. The existing implementation will hardcode the serialization to {} even if the user has subscribed a custom JsonConverter<object> that does something different. I would like to change this behavior for a few reasons:

  • It can be surprising to users: custom object converters cannot be polymorphic with the sole exception of new object() values. Users cannot specify a custom serialization format for new object() values.
  • Keeping it requires maintaining more complex logic in the serialization hot path. The change makes the code simpler by delegating object instance serialization to the relevant converter.
  • It is unlikely that new object() instances are being serialized in production applications.

The breaking change concerns the following scenario:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

var options = new JsonSerializerOptions { Converters = { new CustomObjectConverter() } };
string json = JsonSerializer.Serialize(new object(), options);
Console.WriteLine(json);

public class CustomObjectConverter : JsonConverter<object>
{
    public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) => writer.WriteNumberValue(42);
    public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotSupportedException();
}

In .NET 5 the above would print {}, however under the new changes it would print 42. This could break users with custom object converters who rely on the existing hardcoding behavior.

@eiriktsarpalis eiriktsarpalis added this to the 6.0.0 milestone Jun 18, 2021
@eiriktsarpalis eiriktsarpalis self-assigned this Jun 18, 2021
@ghost
Copy link

ghost commented Jun 18, 2021

Hello @eiriktsarpalis!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 10 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
Copy link

ghost commented Jun 18, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Continuing the backporting efforts of #54420, this PR brings a change from #54328 that requires review in isolation:

Potential breaking change

The changes introduce a deliberate breaking change when it comes to the handling of new object() values. The existing implementation will hardcode the serialization to {} even if the user has subscribed a custom JsonConverter<object> that does something different. I would like to change this behavior for a few reasons:

  • It can be surprising to users: custom object converters cannot be polymorphic with the sole exception of new object() values. Users cannot specify a custom serialization format for new object() values.
  • Keeping it requires maintaining more complex logic in the serialization hot path. The change makes the code simpler by delegating object instance serialization to the relevant converter.
  • It is unlikely that new object() instances are being serialized in production applications.

The breaking change concerns the following scenario:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

var options = new JsonSerializerOptions { Converters = { new CustomObjectConverter() } };
string json = JsonSerializer.Serialize(new object(), options);
Console.WriteLine(json);

public class CustomObjectConverter : JsonConverter<object>
{
    public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) => writer.WriteNumberValue(42);
    public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotSupportedException();
}

In .NET 5 the above would print {}, however under the new changes it would print 42. This could break users with custom object converters who rely on the existing hardcoding behavior.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json, auto-merge
Milestone: 6.0.0

@karelz karelz mentioned this pull request Jun 23, 2021
Closed
@ghost ghost merged commit 707cf4f into dotnet:main Jun 23, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 23, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_exception_i…
ae2a17c 
…nitial_config

* origin/main: (362 commits)
  [wasm][debugger] Reuse debugger-agent on wasm debugger (dotnet#52300)
  Put Crossgen2 in sync with dotnet#54235 (dotnet#54438)
  Move System.Object serialization to ObjectConverter (dotnet#54436)
  Move setting fHasVirtualStaticMethods out of sanity check section (dotnet#54574)
  [wasm] Compile .bc->.o in parallel, before passing to the linker (dotnet#54053)
  Change PathInternal.IsCaseSensitive to a constant (dotnet#54340)
  Make mono_polling_required a public symbol (dotnet#54592)
  Respect EventSource::IsSupported setting in more codepaths (dotnet#51977)
  Root ComActivator for hosting (dotnet#54524)
  Add ILLink annotations to S.D.Common related to DbConnectionStringBuilder (dotnet#54280)
  Fix finalizer issue with regions (dotnet#54550)
  Add support for multi-arch install locations (dotnet#53763)
  Update library testing docs page to reduce confusion (dotnet#54324)
  [FileStream] handle UNC and device paths (dotnet#54483)
  Update NetAnalyzers version (dotnet#54511)
  Added runtime dependency to fix the intermittent test failures (dotnet#54587)
  Disable failing System.Reflection.Tests.ModuleTests.GetMethods (dotnet#54564)
  [wasm] Move AOT builds from `runtime-staging` to `runtime` (dotnet#54577)
  Keep obj node for ArrayIndex. (dotnet#54584)
  Disable another failing MemoryCache test (dotnet#54578)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Jul 23, 2021
@eiriktsarpalis eiriktsarpalis deleted the jsonobjectconverter branch November 9, 2021 21:09
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@layomia layomia layomia approved these changes

@steveharter steveharter Awaiting requested review from steveharter

@jozkee jozkee Awaiting requested review from jozkee

Assignees

@eiriktsarpalis eiriktsarpalis

Labels
area-System.Text.Json
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@eiriktsarpalis @layomia